Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code Quality: Refactor code and make it more modern #1815

Open
wants to merge 87 commits into
base: main
Choose a base branch
from

Conversation

Lamparter
Copy link
Contributor

@Lamparter Lamparter commented Nov 17, 2024

Hello (again again)!
In this PR I've done the following:

  • Run Code Cleanup on the project - remove unnecessary using directives, format documents, etc.
    • Run special advanced code cleanup
    • Sort out .editorconfig
  • Update namespaces of files to be consistent
  • Change folder structure to match a modern project
  • Fix NuGet package sources

...and various other changes that I will add here soon

@Lamparter

This comment was marked as outdated.

@Muny
Copy link

Muny commented Nov 17, 2024

  • Fix NuGet package sources

Can you explain what the purpose of this change is?

@Lamparter
Copy link
Contributor Author

Can you explain what the purpose of this change is?

There was a duplicate entry for the NuGet Gallery server, and the name for MyGet was wrong.

@Muny
Copy link

Muny commented Nov 17, 2024

Can you explain what the purpose of this change is?

There was a duplicate entry for the NuGet Gallery server, and the name for MyGet was wrong.

I guess I'm wondering why MyGet was kept in lieu of NuGet

@Lamparter
Copy link
Contributor Author

I guess I'm wondering why MyGet was kept in lieu of NuGet

The NuGet Gallery server is automatically enabled as a global package source in Visual Studio. Having it inside the nuget.config file would cause it to be duplicated.

@Lamparter
Copy link
Contributor Author

Lamparter commented Nov 25, 2024

Essentially I think it would be best for IronPython to follow a similar structure as the .NET Runtime and Files:

Top-level folder 2nd-level folder Comment
src DLR IronPython.Analyzer IronPython.Modules etc. Should also contain a README.md file
docs This wouldn't exist because of #1812
tests The Tests directory
build Packaging where [Pp]ackaging is the current top-level Package directory
utils The Util directory

In all .NET repos, it's common practice to have build files inside the eng directory. I wonder whether that might be a better name than build

image

@Lamparter
Copy link
Contributor Author

This is why I hate submodules..
about the 5th time I've had to reclone 🫤

@BCSharp
Copy link
Member

BCSharp commented Nov 27, 2024

Personally not a huge fan of console since the IronPythonWindow is basically a console-less version of ipy. What about something like hosts?

Yes, i thought about it too when i realized what @Lamparter means by platforms:

the projects in that folder are the platforms through which you can use IronPython from.

This notion is captured by the term hosts throughout the IronPython/DLR codebase (and perhaps, .NET itself). The only problem with hosts is that ipy, the IronPython compiler, does not fit the bill, and it would be a pity to misplace it in hosts or create a separate directory just for it so that it is not misplaced.

Basically, there are the following projects to sort out:

  • two console hosts (ipy, ipy32)
  • two GUI-less launchers (ipyw, ipyw32)
  • console compiler (ipyc)
  • (hypothetical) GUI host (IDLE-like)

Options:

  1. console:
    • captures ipy, ipy32, ipyc
    • excludes IDLE (gui?)
    • excludes ipyw, ipyw32 (launchers? daemons?)
  2. hosts (a.k.a platforms to some):
    • captures ipy, ipyw, ipy32, ipyw32, IDLE
    • excludes ipyc
  3. executables:
    • captures all of them, and only them; in fact, these projects are the only ones that can be selected as "startup project" in Visual Studio"

My current preferences are (in order):

  1. executables (the longer i think about it)
  2. hosts (if i squint i can put ipyc there; i'm not a fan of platforms in this role)
  3. other (better be non-descriptive than misleadingly descriptive)

@Lamparter
Copy link
Contributor Author

Lamparter commented Nov 27, 2024

Your definition of executables is what I meant by platforms 😄

Perhaps executables is better, though that sounds a lot like where the app binaries would end up in, since some projects set their binary location to exe which is too similar...

@BCSharp
Copy link
Member

BCSharp commented Nov 27, 2024

SdtLib is definitely not an extension and belongs to core.

I had second thoughts about it. Not to put it back in extensions (it doesn't fit there), but perhaps it would be better to promote it one level up. This is a unique part of IronPython since it is actually a part of CPython codebase, only slightly adapted to work around some compatibility issues between the two implementations. Also, it is not an essential part of IronPython — it is to provide library compatibility with CPython, e.g. that 3rd party packages can be imported and run. It is perfectly possible to run IronPython without StdLib, in that case it is just another (dynamic) programming language for .NET. The .NET library is rich enough by itself, if somebody doesn't care about CPython compatibility. Therefore, it is hard to call StdLib "core".

But I am not particularly attached to this idea, placing StdLib in core is good enough. (Let's see how I feel tomorrow. 😄)

@BCSharp
Copy link
Member

BCSharp commented Nov 27, 2024

some projects set their binary location to exe which is too similar...

I hear you. Yes, luckily, we use bin for that. Well, perhaps some more ideas/second thoughts will come up in the coming days.

@slozier
Copy link
Contributor

slozier commented Nov 27, 2024

SdtLib is definitely not an extension and belongs to core.

I had second thoughts about it. Not to put it back in extensions (it doesn't fit there), but perhaps it would be better to promote it one level up. This is a unique part of IronPython since it is actually a part of CPython codebase, only slightly adapted to work around some compatibility issues between the two implementations. Also, it is not an essential part of IronPython — it is to provide library compatibility with CPython, e.g. that 3rd party packages can be imported and run. It is perfectly possible to run IronPython without StdLib, in that case it is just another (dynamic) programming language for .NET. The .NET library is rich enough by itself, if somebody doesn't care about CPython compatibility. Therefore, it is hard to call StdLib "core".

But I am not particularly attached to this idea, placing StdLib in core is good enough. (Let's see how I feel tomorrow. 😄)

Just more food for thought. I'm not sure if we have any tests covering IronPython running without the standard library, but I think it's layered in such a way that it might work without it (and IronPython.Modules?). Note that CPython has been moving features to Python implementations over the years and doesn't even run without the standard library. Whenever that happens we tend to drop the native implementation and use the Python one so maybe we'll get to a point where it's no longer possible to run stand-alone?

@Lamparter
Copy link
Contributor Author

I don't think that's a really good idea...

@BCSharp
Copy link
Member

BCSharp commented Nov 27, 2024

I don't think that's a really good idea...

Can you elaborate?

@BCSharp
Copy link
Member

BCSharp commented Nov 27, 2024

I'm not sure if we have any tests covering IronPython running without the standard library,

Not that I know of.

but I think it's layered in such a way that it might work without it (and IronPython.Modules?).

It used to work, at least a few years ago (IDK about w/o IronPython.Modules), which I discovered accidentaly after forgetting to set IRONPYTHONPATH... There are some things that do not work properly (like that not all code paths can find the built-in codecs), but workarounds are known, issues are opened, and hopefully they will be fixed in the future.

Note that CPython has been moving features to Python implementations over the years and doesn't even run without the standard library.

Good point. Who knows how this will impact IronPython in the future. My favourite 😉 I've seen somewhere in the code (importlib boostrapper?) was a C# string containing a piece of Python code, compiled into the DLL and interpreted at runtime on start. Ingenious.

@Lamparter
Copy link
Contributor Author

Lamparter commented Nov 28, 2024

Can you elaborate?

What's gained in running without the standard library?

@BCSharp
Copy link
Member

BCSharp commented Nov 28, 2024

What's gained in running without the standard library?

  1. Reduced deployment footprint (relevant in resource constrained cases, like e.g. embedded).
  2. Reduced "attack surface" (removed unused code that may contain exploitable bugs; 3.4 is an old library, EOL upstream).

NB IronPython already supports it since the interpreter engine and the standard library are distributed in separate NuGet packages.

@Lamparter
Copy link
Contributor Author

Lamparter commented Nov 29, 2024

I don't understand why the tests are failing 🫤


[net462 cs]    Error Message:
[net462 cs]     Microsoft.Scripting.SyntaxErrorException : Could not find a part of the path 'E:\Repositories\Lamparter\IronPython\Tests\Inputs\raise.py'.
[net462 cs]    Stack Trace:
[net462 cs]       at Microsoft.Scripting.ErrorSink.Add(SourceUnit source, String message, SourceSpan span, Int32 errorCode, Severity severity) in E:\Repositories\Lamparter\IronPython\src\runtime\Src\Microsoft.Scripting\ErrorSink.cs:line 22
[net462 cs]     at IronPython.Compiler.Parser.CreateParser(CompilerContext context, PythonOptions options) in E:\Repositories\Lamparter\IronPython\src\core\IronPython\Compiler\Parser.cs:line 106
[net462 cs]     at IronPython.Runtime.PythonContext.ParseAndBindAst(CompilerContext context) in E:\Repositories\Lamparter\IronPython\src\core\IronPython\Runtime\PythonContext.cs:line 752
[net462 cs]     at IronPython.Runtime.PythonContext.CompilePythonCode(SourceUnit sourceUnit, CompilerOptions options, ErrorSink errorSink) in E:\Repositories\Lamparter\IronPython\src\core\IronPython\Runtime\PythonContext.cs:line 807
[net462 cs]     at IronPython.Runtime.PythonContext.CompileSourceCode(SourceUnit sourceUnit, CompilerOptions options, ErrorSink errorSink) in E:\Repositories\Lamparter\IronPython\src\core\IronPython\Runtime\PythonContext.cs:line 816
[net462 cs]     at Microsoft.Scripting.SourceUnit.Execute(Scope scope, ErrorSink errorSink) in E:\Repositories\Lamparter\IronPython\src\runtime\Src\Microsoft.Scripting\SourceUnit.cs:line 217
[net462 cs]     at Microsoft.Scripting.Hosting.ScriptSource.Execute(ScriptScope scope) in E:\Repositories\Lamparter\IronPython\src\runtime\Src\Microsoft.Scripting\Hosting\ScriptSource.cs:line 107
[net462 cs]     at Microsoft.Scripting.Hosting.ScriptEngine.ExecuteFile(String path, ScriptScope scope) in E:\Repositories\Lamparter\IronPython\src\runtime\Src\Microsoft.Scripting\Hosting\ScriptEngine.cs:line 150
[net462 cs]     at IronPythonTest.EngineTest.TestLineInfo(ScriptScope scope, String lineNumber) in E:\Repositories\Lamparter\IronPython\tests\ironpython\EngineTest.cs:line 2370
[net462 cs]     at IronPythonTest.EngineTest.ScenarioStackFrameLineInfo() in E:\Repositories\Lamparter\IronPython\tests\ironpython\EngineTest.cs:line 2355

doesn't make a difference 😕
@Lamparter
Copy link
Contributor Author

Could not find a part of the path 'E:\Repositories\Lamparter\IronPython\Tests\Inputs\raise.py'

This path isn't specified anywhere, and the source code literally directs the tests to tests\python\Inputs and not Tests - I have no idea where it's getting this path from.

@slozier
Copy link
Contributor

slozier commented Nov 29, 2024

Could not find a part of the path 'E:\Repositories\Lamparter\IronPython\Tests\Inputs\raise.py'

This path isn't specified anywhere, and the source code literally directs the tests to tests\python\Inputs and not Tests - I have no idea where it's getting this path from.

This one maybe?
https://github.com/Lamparter/IronPython/blob/refactor/tests/ironpython/Cases/IronPythonCases.cs

Slozier is smart
@slozier
Copy link
Contributor

slozier commented Nov 29, 2024

A few comments:

Test issue:

         public TestManifest(Type parent) {
-            var file = parent.Assembly.GetManifestResourceStream($"IronPythonTest.Cases.{parent.Name}Manifest.ini");
+            var file = parent.Assembly.GetManifestResourceStream($"IronPython.Tests.Cases.{parent.Name}Manifest.ini");

There's a bunch of files which had their perms altered from 100755 → 100644 (like ipy3.4, dos2unix.sh, etc.). Having proper permissions on these scripts is important.

.gitignore: I definitely do not like the generic catch all of every conceivable generated file types. Please remove the additions.

Not a fan of the tests folder structure. Maybe something like:

  • _ctypes_test
  • IronPython.Tests
  • suite (not sure I really like this one, but better than python)... or cases?

Otherwise it feels like it's almost there. Unfortunately I still can't review with GH because of the 3k file limit. Maybe we could split it up into smaller PRs, for example one with the non-src changes and then src? Guess I need to start looking at merging main into 3.6 before the refactoring...

@Lamparter
Copy link
Contributor Author

Got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants